Repository navigation
TIME (time64[ns]) type unhandled (#505) - #508
peco-engineer-bot[bot] wants to merge 5 commits into
Conversation
![peco-engineer-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-targeted fix for the unhandled time64[ns] TIME type — the new Time32/Time64 arms, container, and unit tests are coherent and release arrays correctly. One medium concern: .In(tvc.location) shifts a zoneless SQL TIME when a non-UTC WithLocation is configured, contradicting the container's stated intent and untested by the UTC-only unit tests.
Addresses: - #4332939800 at internal/rows/arrowbased/columnValues.go:531 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good — the TIME fix is correct and well-tested (unit→ns math is sound, the zoneless-UTC rendering is deliberate and matches the sibling date/timestamp containers, and the Time32/Time64 unit cases are covered). One medium: the kernel/SEA backend (internal/arrowscan) still errors on TIME, so the two backends now diverge for the same query despite the repo's cross-backend parity contract. Note also that a scanned TIME carries a spurious 1970-01-01 date (documented in the container comment, and the only reasonable anchor), which callers using .Format/.Date() will observe.
Addresses: - #4333004011 at internal/rows/arrowbased/arrowRows.go:654 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the time32/time64 TIME handling is correct, overflow-safe, lifecycle-consistent with the existing containers, and the Thrift and kernel paths render identically (parity test extended). One low note: a zoneless TIME is epoch-anchored on 1970-01-01, which would surface a spurious date if a TIME is ever rendered nested inside a complex type via marshal() — likely unreachable and consistent with existing nested-TIMESTAMP behavior.
Addresses: - #4333050880 at internal/rows/arrowbased/columnValues.go:518 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
One cross-backend concern: the fix special-cases nested TIME rendering on the Thrift side (marshalScalar → time-of-day string) but not on the kernel side (internal/arrowscan nested path still emits the full time.Time.String() with the 1970-01-01 epoch date), so a TIME inside a STRUCT/LIST/MAP diverges between the two backends — and no parity test covers the nested case. Top-level handling and the unit tests look correct.
Addresses: - #4333081459 at internal/arrowscan/arrowscan.go:225 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Summary
Automated fix for #505 — TIME (time64[ns]) type unhandled.
Added *arrow.Time32Type/*arrow.Time64Type arms to makeColumnValueContainer plus a new timeValueContainer (and timeUnitToNanos helper) that renders units-since-midnight as a UTC wall-clock time.Time; this resolves the "unhandled type time64[ns]" failure for SQL TIME columns. Unit tests pass and the full
make testsuite +make lintare green. The e2e test (TestE2ETimeTypeScan) was confirmed red against the live warehouse in the prior phase; it only skips here because this runner's bash subprocess scrubs credential-shaped env vars so the warehouse creds aren't reachable from the tool.Root cause & plan
Root cause: On the Thrift Arrow path, a TIME column arrives in the Arrow IPC schema as time64[ns] (microsecond-precision TIME is still carried as time64 nanoseconds). arrowValueContainerMaker.makeColumnValueContainer (internal/rows/arrowbased/arrowRows.go, ~line 591+) switches on the concrete arrow.DataType and has arms for Date32/Timestamp/Decimal/String/List/Map/Struct/Null etc., but NO arm for *arrow.Time32Type or *arrow.Time64Type. The value hits the default arm and returns errArrowRowsUnhandledArrowType(t.String()) -> "databricks: arrow row scanner unhandled type time64[ns]". Because containers are built eagerly in makeColumnValuesContainers when the row scanner is created, this fails the whole result set before any row is returned. DATE and TIMESTAMP already have arms (dateValueContainer / timestampValueContainer); TIME has none.
Files:
internal/rows/arrowbased/arrowRows.go,internal/rows/arrowbased/columnValues.go,driver_e2e_test.goPlanned coverage:
SELECT TIME '09:15:00.123456'must run and rows.Scan must succeed (no 'unhandled type time64[ns]' error), yielding a time-of-day time.Time with Hour()=9, Minute()=15, Second()=0, Nanosecond()=123456000 and UTC location (no date, no location). This is the exact reproducer from the issue and exercises the real server's time64[ns] encoding end-to-end. (time64 (primary, live e2e))Files changed
internal/rows/arrowbased/time_test.godriver_e2e_test.gointernal/rows/arrowbased/columnValues.gointernal/rows/arrowbased/arrowRows.goTest plan
TestTime64ValueContainer (internal/rows/arrowbased/time_test.go)— fails (red) against the original code, passes (green) after the fixTestTime32ValueContainer (internal/rows/arrowbased/time_test.go)— fails (red) against the original code, passes (green) after the fixTestE2ETimeTypeScan (driver_e2e_test.go)— fails (red) against the original code, passes (green) after the fix🤖 Generated by engineer-bot (bug-fix flow) — review before merge.